-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Release 2.0.0 #57
Release 2.0.0 #57
Conversation
3ff3952
to
fdf4711
Compare
4b976fc
to
bb83273
Compare
tests/transfomers/portable-text-transformer/mapi-transformer.spec.ts
Outdated
Show resolved
Hide resolved
7f66ffa
to
97adf07
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
- Coverage 94.79% 88.80% -5.99%
==========================================
Files 19 22 +3
Lines 365 420 +55
Branches 68 85 +17
==========================================
+ Hits 346 373 +27
- Misses 19 47 +28 ☔ View full report in Codecov by Sentry. |
_key: "guid", | ||
component: { | ||
_ref: "linkedItemOrComponentCodename", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pokornyd , Would it be possible to change the _ref
and _key
in various objects with the expected value? I.e. in this case you assign a codename
, but when using this I have no way of knowing if it's a codename/id or something else entirely.
If these properties are necessary for some reason and can't be renamed, I think it would be worth adding new custom properties that will represent the ID/codename appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @Enngage. this had already been done. the underlying Reference
type now also contains a referenceType
string literal, with value of codename
, external-id
or id
, informing you of the nature of the value stored in _ref
.
the showcase is updated to reflect that, but the readme embed points to main branch version of it. should get sorted after the merge :)
|
||
const components: PortableTextComponents = { | ||
types: { | ||
image: ({ value }: PortableTextComponentProps<PortableTextImage>) => resolveImage(value, h, toVueImageDefault), | ||
image: ({ value }: PortableTextComponentProps<PortableTextImage>) => resolveImage(value, h), | ||
table: ({ value }: PortableTextComponentProps<PortableTextTable>) => resolveTable(value, h, toPlainText), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pokornyd not sure if this was changed, but when testing it I noticed that all properties under types
were required - meaning you had to provide some sort of implementation even if you only needed to provide a resolver for image. Can these props be made optional instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the helper types (PortableTextHtmlResolvers
, PortableTextReactResolvers
) are both set to be partial. I didn't create helper type for Vue, but PortableTextComponents["types"]
appears to be partial by default
src/utils/transformer-utils.ts
Outdated
(child: T) => traversePortableText(child, callback), | ||
); | ||
} | ||
export const traversePortableText = < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: formatting of generic can be onelined
import { DomHtmlNode, DomNode } from "../../parser/parser-models.js"; | ||
|
||
export type NodeToHtml<TContext = unknown> = ( | ||
node: DomHtmlNode<any>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't this be node: DomHtmlNode<unknown>
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep. changed
Motivation
Checklist
How to test
If manual testing is required, what are the steps?